Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

rust/src/bwrap.rs: check for fuse version #5074

Closed
wants to merge 1 commit into from
Closed

rust/src/bwrap.rs: check for fuse version #5074

wants to merge 1 commit into from

Conversation

jmarrero
Copy link
Member

@jmarrero jmarrero commented Aug 29, 2024 •
edited
Loading

We have been building with fuse but changed to fuse3 on:
https://src.fedoraproject.org/rpms/rpm-ostree/c/3c602a23787fd2df873c0b18df3133c9fec4b66a
However our code is just calling fuse's fusermount.
We are updating our spec and code based on the discusion on:
#5047

Co-authored-by: @cgwalters

@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Aug 29, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

cgwalters
cgwalters reviewed Aug 29, 2024
View reviewed changes
build.rs Outdated
@@ -16,6 +32,29 @@ fn detect_fedora_feature() -> Result<()> {
Ok(())
}

fn detect_el9_or_older() -> Result<()> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not opposed to doing it this way but I think it will actually be cleaner to:

  • Add a fuse3 feature on the rust side
  • Change the spec file to enable it

Look at how this is done for the RHSM stuff in the spec:
%{?with_rhsm:--enable-featuresrs=rhsm}

Ugh well...I guess we'd need to concatenate it in the spec file...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh I see, yeah that makes more sense.

cgwalters
cgwalters reviewed Aug 29, 2024
View reviewed changes
rust/src/bwrap.rs Outdated
Comment on lines 125 to 128
let mut fuse_command = "fusermount3";

#[cfg(feature = "el_9_or_older")]
fuse_command = "fusermount";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

   let fuse_command = if cfg!(feature = "el_9_or_older")) { "fusermount" } else {"fusermount3" };

cgwalters
cgwalters reviewed Aug 29, 2024
View reviewed changes
build.rs Outdated
Comment on lines 6 to 7
if let Some(pos) = platform_id.find("el") {
let version_part = &platform_id[pos + 2..]; // Skip "el"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try using platform_id.split_once("el")

@cgwalters
Copy link
Member

Ugh sorry there's something I just realized now too...in theory we should support other distributions and who knows how they have fuse set up.

I keep circling back to...really, either of fusermount or fusermount3 should work so we basically probe dynamically at runtime and use fusermount3 if it exists first.

@cgwalters
Copy link
Member

But to do that we need to know that we can run fusermount3 against a fuse mount created with fuse2, and I am 90% sure that's the case...looking

@cgwalters
Copy link
Member

quick only compile tested function to do the dynamic probing

fn get_fusermount_path() -> Result<Utf8PathBuf> {
    let path = std::env::var("PATH").expect("PATH set");
    let fusermount_binaries = ["fusermount", "fusermount3"];
    for elt in path.split(':').map(Utf8Path::new) {
        for bin in fusermount_binaries {
            let target = elt.join(bin);
            if target.try_exists()? {
                return Ok(target);
            }
        }
    }
    anyhow::bail!("No fusermount path found")
}

@jmarrero
Copy link
Member Author

Cool, I'll update the PR then and drop the build conditional.

@jmarrero jmarrero changed the title WIP: Build conditional WIP: rust/src/bwrap.rs: check for fuse version Aug 30, 2024
HuijingHei
HuijingHei approved these changes Aug 30, 2024
View reviewed changes
Copy link
Member

@HuijingHei HuijingHei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, need to fix the minor codestyle error.

@jmarrero @cgwalters
rust/src/bwrap.rs: check for fuse version
5641308 
We have been building with fuse but changed to fuse3  on:
https://src.fedoraproject.org/rpms/rpm-ostree/c/3c602a23787fd2df873c0b18df3133c9fec4b66a
However our code is just calling fuse's fusermount.
We are updating our spec and code based on the discusion on:
coreos#5047

Co-authored-by: Colin Walters <[email protected]>
@jmarrero jmarrero marked this pull request as ready for review August 30, 2024 13:23
@jmarrero jmarrero changed the title WIP: rust/src/bwrap.rs: check for fuse version rust/src/bwrap.rs: check for fuse version Aug 30, 2024
@openshift-ci OpenShift CI
Copy link

openshift-ci bot commented Aug 30, 2024

@jmarrero: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/fcos-e2e 5641308 link true /test fcos-e2e

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

This was referenced Sep 2, 2024
Merged
Merged
cgwalters
cgwalters approved these changes Sep 3, 2024
View reviewed changes
rust/src/bwrap.rs
@@ -122,7 +138,7 @@ impl Drop for RoFilesMount {
return;
};
// We need to unmount before letting the tempdir cleanup run.
let success = Command::new("fusermount")
let success = Command::new(get_fusermount_path().unwrap().to_string())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could factor this out into a helper that we run once at startup, that would help us avoid the unwrap() but I'm fine with this as is too

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's see if this fixes CI with the spec patch and I'll do a follow up with the startup part?

I guess it would go here

rpm-ostree/rust/src/daemon.rs

Line 25 in cd6fd88

/// Validate basic assumptions on daemon startup.
?

@cgwalters
Copy link
Member

I think we need to roll this PR together with #5047 to get the FCOS CI to work

@jmarrero
Copy link
Member Author

jmarrero commented Sep 3, 2024

This now is incorporated into #5047

@jmarrero jmarrero closed this Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HuijingHei HuijingHei HuijingHei approved these changes

@cgwalters cgwalters cgwalters approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jmarrero @cgwalters @HuijingHei